[fix](be) Harden JNI reader split lifecycle#65503
Merged
Gabriel39 merged 5 commits intoJul 13, 2026
Merged
Conversation
### What problem does this PR solve? JNI-backed table readers could violate scanner selection, cancellation, cleanup, EOF, adaptive batching, and rolling-upgrade contracts. Issue Number: N/A Related PR: N/A ### Release note Harden JNI table-reader lifecycle handling and compatibility for Paimon and Hudi scans. ### Check List - [x] Added unit tests for the fixes - [x] Added comments for compatibility and lifecycle invariants - [x] Passed 45 focused ASAN BE unit tests - [x] Passed repository format and diff checks
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 5:19 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 5:19 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 30004 ms |
Contributor
TPC-DS: Total hot run time: 180273 ms |
Contributor
ClickBench: Total hot run time: 24.99 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review found three blocking issues that should be fixed before merge:
- The PAIMON_CPP FileScannerV2 exclusion does not match real FE Paimon scan params, so PAIMON_CPP JNI splits can still be routed into V2.
- Failed JNI Java cleanup is made non-retryable even though
JniTableReader::close()leaves_closedfalse after failure. - Adaptive batch sizing reaches Paimon JNI after Java open, too late to cap Paimon's first physical
readBatch()size.
Checkpoint conclusions: the PR goal is only partially met because the lifecycle hardening and routing fixes still miss real execution paths. The change is focused, but the tests use simplified shapes that miss the FE-generated Paimon params, the real close-reset state transition, and Paimon's open-time batch option. No new lock-order or transaction/persistence issue was found. Mixed-version Paimon option fallback looks correct. Observability changes are not required, but M2 is a memory/performance correctness issue for the adaptive batch feature. Validation was static only, as the review prompt forbids builds and test execution.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65503 Problem Summary: Paimon C++ splits could still enter FileScannerV2 because scanner selection inspected a scan-level table-format descriptor that FE only sends per split. Failed JNI cleanup also discarded the Java object before a retry, and adaptive JNI sizing was applied after Paimon had already opened its physical reader. Use the scan-level Paimon predicate marker for routing, retain JNI state until cleanup succeeds, and seed adaptive batch sizing before split preparation opens JNI readers. ### Release note Ensure Paimon C++ scans use the supported scanner path, JNI cleanup remains retryable, and the first Paimon JNI batch honors adaptive sizing. ### Check List (For Author) - Test: Unit Test - Remote ASAN BE unit tests: FileScannerV2Test.PaimonCppReaderForcesLegacyScanner and JniTableReaderTest.* (6 tests passed) - Remote build-support/check-format.sh passed with clang-format 16 - Remote ASAN_UT changed objects compiled successfully - Remote clang-tidy analyzers completed for changed production files; final wrapper summary was not captured after the SSH output channel ended - Behavior changed: Yes, scanner routing, JNI cleanup retry state, and first-batch sizing are corrected - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Review completed after 2 rounds. All five subagents converged on this final M1/M2 comment set with NO_NEW_VALUABLE_FINDINGS.
I found two blocking issues: one compatibility gap in the Paimon CPP V1/V2 selection, and one lifecycle gap where the new JNI close retry state cannot be reached through the scanner close path. I did not duplicate existing review threads for the prior first-Paimon-JNI-batch timing concern or the inner JNI close-state retryability thread.
Critical checkpoint conclusions:
- Goal: the PR addresses current-FE Paimon selection, JNI cleanup, cancellation, and adaptive batch feedback, but still misses rolling-upgrade Paimon CPP split-level metadata and scanner-level close retry.
- Scope: mostly focused, but the close retry fix crosses the
JniTableReader/FileScannerV2lifecycle boundary. - Concurrency: no new shared-thread synchronization was added; the issue is lifecycle state ordering during close.
- Lifecycle: not complete;
FileScannerV2marks itself closed before retained JNI cleanup state can be retried. - Configuration: no new config.
- Compatibility: not complete for old FE/new BE Paimon CPP shapes that keep Paimon metadata only at split level.
- Parallel paths: Paimon/Hudi hybrid batch forwarding was reviewed; no additional adaptive-batch issue found after the current seeding change.
- Tests: new BE tests are discovered by the existing CMake glob, but coverage is still missing for the accepted Paimon CPP rolling-upgrade selector and scanner-level failing-then-succeeding close path.
- Observability/performance: warning/profile changes and adaptive-batch seeding look reasonable; no separate metrics issue found.
- Transactions/persistence/data writes: not directly involved.
- User focus: no additional user-provided review focus.
Validation: static review only per the review prompt; no builds or unit tests were run. git diff --check is clean in this checkout.
| // FE stores Paimon's table-format descriptor per split, but paimon_predicate is a scan-level | ||
| // Paimon marker. PAIMON_CPP is also selected per split, and FileScannerV2 cannot dispatch that | ||
| // reader type, so retain the V1 path until the V2 hybrid reader supports it. | ||
| const bool uses_paimon_cpp_reader = scan_params.__isset.paimon_predicate && |
Contributor
There was a problem hiding this comment.
The exclusion still misses the rolling-upgrade shape where Paimon metadata is only on each split. This selector only sees TFileScanRangeParams, so an old FE that has enable_paimon_cpp_reader=true but still sends Paimon predicate/reader metadata only in TFileRangeDesc.table_format_params.paimon_params will leave scan_params.__isset.paimon_predicate false and V2 remains selected. The first FORMAT_JNI range with TPaimonReaderType::PAIMON_CPP is not supported by V2: FileScannerV2::is_supported() accepts Paimon JNI only for PAIMON_JNI, and PaimonHybridReader dispatches only PAIMON_JNI to the JNI child. Please either use a marker that is guaranteed for both current and rolling-upgrade Paimon CPP scans, or conservatively keep JNI Paimon CPP on V1 until the split-level reader type can be inspected, and add a test for scan params without scan-level paimon_predicate plus a split-level PAIMON_CPP range.
Contributor
TPC-H: Total hot run time: 29530 ms |
Contributor
TPC-DS: Total hot run time: 181365 ms |
Contributor
ClickBench: Total hot run time: 25.09 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#65503 Problem Summary: FileScannerV2 selection could still route rolling-upgrade Paimon CPP JNI splits into an unsupported V2 reader, and a failed table-reader close left the scanner marked closed so retained JNI cleanup state could not be retried. Conservatively keep JNI scans on V1 while the Paimon C++ reader option is enabled, and roll back the scanner close guard when table-reader cleanup fails. ### Release note Harden Paimon CPP scanner selection and retry JNI cleanup after close failures. ### Check List (For Author) - Test: Unit Test - FileScannerV2Test.* and JniTableReaderTest.*: 20 tests passed under ASAN BE UT - Behavior changed: Yes. Unsupported Paimon CPP JNI splits stay on V1, and failed V2 table-reader cleanup can be retried. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes based on static review.
Critical checkpoint conclusions:
- Goal: the PR addresses several prior JNI/FileScannerV2 review issues, but three compatibility/lifecycle/resource issues remain.
- Scope: the changes are focused on scanner selection, JNI cleanup, adaptive batch sizing, hybrid readers, and BE tests.
- Concurrency/lifecycle: scanner close retry and JNI resource lifecycle were reviewed; Paimon Java cleanup still invalidates retry state.
- Compatibility: rolling-upgrade Paimon JNI without reader_type is still not preserved on the V2 path.
- Parallel paths: FileScannerV1, FileScannerV2, Paimon/Hudi hybrid readers, and representative Java JNI scanners were checked.
- Testing: new BE tests cover useful cases, but they miss the three accepted edge cases below.
- Configuration/storage/transactions/observability: no new config, storage format, transaction, or observability requirement was found.
User focus: no additional user-provided focus was supplied.
Validation: static review only. I did not run builds or tests because the review prompt forbids build/test operations in this runner; the checkout also lacks .worktree_initialized and thirdparty/installed.
### What problem does this PR solve? Issue Number: None Related PR: apache#65503 Problem Summary: Legacy Paimon JNI splits without reader_type could still enter FileScannerV2, adaptive batch-size updates after open did not resize Paimon physical readers, and failed Java cleanup cleared resource references before a retry. Keep JNI compatibility shapes on the legacy scanner, preserve the initial Paimon physical batch size after open, and retain each Java resource until its cleanup succeeds. ### Release note Harden legacy JNI selection, Paimon batch-size handling, and retryable Java cleanup. ### Check List (For Author) - Test: Unit Test - FileScannerV2Test.*, JniTableReaderTest.*, and PaimonJniReaderTest.*: 30 tests passed under ASAN BE UT - PaimonJniScannerTest: 10 tests passed; checkstyle reported 0 violations - clang-format and git diff checks passed - Behavior changed: Yes. Legacy JNI compatibility splits stay on V1, Paimon ignores unsupported post-open physical batch resizing, and failed Java cleanup remains retryable. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review completed for PR 65503.
I found one remaining issue worth requesting changes for: retryable JNI close failures can double-publish the same split profile counters/statistics before a later close succeeds.
Checkpoint summary:
- Goal/test evidence: the PR addresses JNI scanner lifecycle, FileScannerV2 selection compatibility, adaptive batch sizing, and Paimon Java cleanup retry behavior with focused BE and Java tests in the patch.
- Scope/focus: the change is focused on scanner/JNI lifecycle and compatibility paths; the broad scan-level FORMAT_JNI fallback was reviewed and treated as a conservative fallback because V1 has matching non-Paimon JNI reader coverage and adaptive batch forwarding.
- Concurrency/lifecycle: close retry state was reviewed through FileScannerV2, JniTableReader, hybrid readers, and Paimon Java resources. The resource-retention path is now retryable, but the profile-publication side effect below remains.
- Compatibility/parallel paths: Paimon scan-level versus split-level predicate/options/Hadoop-conf fallback was compared against the legacy V1 reader. The plugin-driven no-filter Paimon predicate omission found during review is in unchanged provider code and is out of this PR's patch scope.
- Tests: added tests cover the main selector, retry, EOF/cancellation, adaptive batch, and Paimon fallback paths. I did not run builds/tests in this review runner, per the review prompt; validation was static. The saved patch applies cleanly in reverse with whitespace checking.
- User focus: no additional user-provided review focus was supplied.
All round-1 subagents and risk-focused subagents converged on this exact final comment set with NO_NEW_VALUABLE_FINDINGS.
### What problem does this PR solve? Issue Number: None Related PR: apache#65503 Problem Summary: A retryable JNI scanner close published the same split profile on every cleanup attempt because the Java scanner and watcher values must remain intact after a failed close. Reserve profile publication per split so close retries only repeat resource cleanup, then reset that publication state when the split is successfully released. ### Release note Keep JNI split profile counters idempotent across close retries. ### Check List (For Author) - Test: Unit Test - FileScannerV2Test.*, JniTableReaderTest.*, and PaimonJniReaderTest.*: 30 tests passed under ASAN BE UT - FailedCloseCanBeRetried asserts the close profile counter is unchanged after a fail-then-success retry - check-format and git diff checks passed - Behavior changed: Yes. Retried JNI cleanup no longer publishes the same split profile more than once. - Does this need documentation: No
Contributor
TPC-H: Total hot run time: 29512 ms |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29489 ms |
Contributor
There was a problem hiding this comment.
I found one remaining issue in the Paimon JNI cleanup retry path.
The scanner-selection changes now keep scan-level FORMAT_JNI on the legacy path, which covers the Paimon CPP and missing-reader_type compatibility shapes already discussed in existing review threads. The C++ JNI close retry path now preserves JNI state, gates split profile publication, and reopens FileScannerV2 close for explicit retry. Adaptive batch sizing is seeded before eager JNI open and propagated through Paimon/Hudi hybrid children, with Paimon post-open resizing intentionally left unchanged.
Remaining issue: the Java Paimon close path preserves retryable resources after a failed cleanup, but still decrements the active-scanner gauge during that failed close. Please fix that before merging.
Validation: static review only. The PR head still matches c05c7b5. The checkout is shallow, so full local range checks and builds/tests were not run here. Added-line whitespace in the authoritative patch was checked, and git show --check HEAD reported no whitespace problems for the checked-out commit.
Review checkpoints: goal is mostly met but one observability/lifecycle issue remains; scope is focused; lifecycle and retry behavior were reviewed; no new configs or persistence formats; compatibility paths were checked against current and legacy Paimon shapes; tests cover most retry/routing/adaptive paths but miss the active-scanner metric assertion.
Contributor
TPC-DS: Total hot run time: 182194 ms |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
ClickBench: Total hot run time: 25.04 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
approved these changes
Jul 13, 2026
hello-stephen
pushed a commit
that referenced
this pull request
Jul 13, 2026
### What problem does this PR solve? Fix a BE UT compile failure introduced by the combination of #65500 and #65503. `SplitReadOptions` now has `conjuncts` and `all_runtime_filters_applied`, while the new `jni_table_reader_test.cpp` designated initializer did not initialize them. With `-Werror,-Wmissing-designated-field-initializers`, this fails compilation. This PR explicitly initializes the two fields in the test and adds the missing `<optional>` include for `std::nullopt`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
JNI-backed file scans had several lifecycle, compatibility, and adaptive-batching gaps:
reader_type—to a scanner that cannot dispatch them.This PR hardens those paths by:
Release note
Harden JNI table-reader lifecycle handling, adaptive batching, and rolling-upgrade compatibility for Paimon and Hudi scans.
Check List
FileScannerV2Test.*,JniTableReaderTest.*, andPaimonJniReaderTest.*after addressing the latest review.PaimonJniScannerTestJava tests; checkstyle reported 0 violations.build-support/check-format.shandgit diff --check.stddef.hand pre-existing repository diagnostics.